Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUM 6569 clean #2089

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

RUM 6569 clean #2089

wants to merge 4 commits into from

Conversation

mariedm
Copy link
Contributor

@mariedm mariedm commented Oct 23, 2024

What and why?

What and why?

This PR builds on the work of #2058 and #2067, which introduced Fine-Grained Masking (FGM) overrides (see the RFC [internal] for more details). It implements the logic for applying touch privacy overrides at the view level, allowing views to have specific privacy settings that take precedence over global configurations.

How?

  • The global touch privacy level is now passed down to the WindowTouchSnapshotProducer.
  • When a touch is detected (in notify_sendEvent), the applicable privacy setting is resolved, considering both view-level overrides and the global setting.
  • The overrideForTouch dictionary is used to track privacy overrides for each ongoing touch event, ensuring the resolved privacy setting is reused throughout the lifecycle of the touch (from began to ended).
  • Touch events are recorded or ignored based on the resolved privacy setting, with the view-level override taking precedence over the global configuration.

🧪 Note: A flaky test in SessionReplayOverrideTests was also identified and fixed in this PR.

👩‍🎨 Design Consideration: An alternative solution considered was to store the Recorder.Context within the WindowTouchSnapshotProducer, injected during initialization, and updated from the recorder when necessary. This stored context would then be used in both takeSnapshot and notify_sendEvent. However, this approach introduced complexity in managing the context state inside the snapshot producer, with potential issues around timing, synchronization, and the risk of stale contexts being used.

Another alternative was to pass the entire recording context directly into notify_sendEvent, but this was not ideal since touch event processing only requires the global touch privacy, not the full recording context.

The design decision was made to pass the global touch privacy during initialization because it is static and does not change during the session, meaning it only needs to be provided once. In contrast, the recording context can change and is therefore passed to takeSnapshot as needed, ensuring the correct context is used at the time of snapshot capture.

📈 Performance Impact Note: Running the Shopist application [internal] on an iPhone 15, I measured the performance impact of notify_sendEvent of my feature branch (mariedm/FGM-overrides) compared to the current branch. The results are as follows:

  • Current branch: 52 μs on average
  • Feature branch: 72.92 μs on average

While this indicates a 40% slowdown, the absolute difference is just 20 μs, which is unlikely to be noticeable in most real-world scenarios. In most applications, an additional 20 μs per touch event is unlikely to affect performance in a meaningful way.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes
  • Add Objective-C interface for public APIs (see our guidelines (internal)) and run make api-surface

How?

A brief description of implementation details of this PR.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes
  • Add Objective-C interface for public APIs (see our guidelines (internal)) and run make api-surface

@mariedm mariedm changed the base branch from develop to mariedm/FGM-overrides October 23, 2024 17:26
@datadog-datadog-prod-us1
Copy link

Datadog Report

Branch report: RUM-6569-clean
Commit report: e2164dc
Test service: dd-sdk-ios

✅ 0 Failed, 1582 Passed, 0 Skipped, 1m 2.06s Total Time
🔻 Test Sessions change in coverage: 2 decreased, 1 increased (+0.04%), 3 no change

🔻 Code Coverage Decreases vs Default Branch (2)

  • test DatadogInternalTests tvOS 79.72% (-0.05%) - Details
  • test DatadogCrashReportingTests tvOS 26.77% (-0.02%) - Details

Base automatically changed from mariedm/FGM-overrides to develop October 24, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant